feat: add support for trigger comments, trigger enabled/disabled state, and sequence comments#479
Open
ayusssmaan wants to merge 3 commits into
Open
Conversation
…e, and sequence comments - Trigger comments: inspect COMMENT ON TRIGGER via pg_description, emit COMMENT ON TRIGGER DDL on diff - Trigger state: inspect trigger enabled flag (pg_trigger.tgenabled), emit ALTER TABLE ENABLE/DISABLE TRIGGER on diff - Sequence comments: inspect COMMENT ON SEQUENCE via pg_description, emit COMMENT ON SEQUENCE DDL on diff - Test fixtures: add_trigger_comment, add_sequence_comment, disable_trigger (all passing)
…CREATE TABLE When a table with BIGSERIAL/SERIAL columns was created for the first time, pgschema skipped the sequence from addedSequences (correctly, since CREATE TABLE creates it implicitly), but this also skipped emitting any COMMENT ON SEQUENCE declared in the model. On the second run, the sequence existed in the live DB and the comment diff fired, re-applying all sequence comments indefinitely. Fix: track skipped-but-commented SERIAL sequences in addedSerialSeqComments and emit their COMMENT ON SEQUENCE statements after all CREATE TABLE calls complete. Test: add_serial_sequence_comment_on_create fixture verifies COMMENT is emitted on first deploy alongside CREATE TABLE, not deferred to a subsequent run.
Greptile SummaryThis PR teaches pgschema to track more trigger and sequence metadata. The main changes are:
Confidence Score: 2/5This should be fixed before merging.
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile fixes: - ir/ir.go: rename Enabled bool → Disabled bool (omitempty) so zero-value means enabled, matching Postgres default; avoids spurious DISABLE TRIGGER on triggers that omit the field - ir/inspector.go: set Disabled=true only when tgenabled='D' - internal/diff/trigger.go: update all !trigger.Enabled → trigger.Disabled; add DISABLE TRIGGER emission to view trigger create path (was missing) - internal/diff/table.go: update Enabled → Disabled in trigger diff detection - internal/diff/view.go: emit trigger comment and disabled state in both constraint-recreate and non-constraint modify paths for view triggers CI fix: - Add plan.sql, plan.json, plan.txt for all 4 new fixtures so TestPlanAndApply passes
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing schema drift detection and DDL generation for trigger comments, trigger enabled/disabled state, and sequence comments, plus fixes idempotency for comments on SERIAL-owned sequences created implicitly by CREATE TABLE.
Changes:
- Extend IR + inspector queries to load trigger comments, trigger
tgenabledstate, and sequence comments from PostgreSQL catalogs. - Diff and emit
COMMENT ON TRIGGER,ALTER TABLE ENABLE/DISABLE TRIGGER, andCOMMENT ON SEQUENCEstatements. - Fix SERIAL sequence comment idempotency by emitting comments for implicitly created sequences after table creation.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/diff/create_trigger/disable_trigger/plan.txt | New fixture asserting trigger disable shows up in plan output. |
| testdata/diff/create_trigger/disable_trigger/plan.sql | New fixture asserting ALTER TABLE ... DISABLE TRIGGER SQL output. |
| testdata/diff/create_trigger/disable_trigger/plan.json | New fixture asserting JSON step metadata for disabling a trigger. |
| testdata/diff/create_trigger/disable_trigger/old.sql | Fixture “before” schema for trigger disable case. |
| testdata/diff/create_trigger/disable_trigger/new.sql | Fixture “after” schema including disabled trigger. |
| testdata/diff/create_trigger/disable_trigger/diff.sql | Fixture expected diff SQL for disabling a trigger. |
| testdata/diff/comment/add_trigger_comment/plan.txt | New fixture asserting trigger comment appears in plan output. |
| testdata/diff/comment/add_trigger_comment/plan.sql | New fixture asserting COMMENT ON TRIGGER SQL output. |
| testdata/diff/comment/add_trigger_comment/plan.json | New fixture asserting JSON step metadata for trigger comment. |
| testdata/diff/comment/add_trigger_comment/old.sql | Fixture “before” schema for trigger comment case. |
| testdata/diff/comment/add_trigger_comment/new.sql | Fixture “after” schema including trigger comment. |
| testdata/diff/comment/add_trigger_comment/diff.sql | Fixture expected diff SQL for trigger comment. |
| testdata/diff/comment/add_serial_sequence_comment_on_create/plan.txt | New fixture covering SERIAL-owned sequence comment emission on initial create. |
| testdata/diff/comment/add_serial_sequence_comment_on_create/plan.sql | New fixture asserting comment emitted after CREATE TABLE for SERIAL sequence. |
| testdata/diff/comment/add_serial_sequence_comment_on_create/plan.json | New fixture asserting JSON step metadata for SERIAL sequence comment. |
| testdata/diff/comment/add_serial_sequence_comment_on_create/old.sql | Fixture “before” schema (empty) for SERIAL sequence comment case. |
| testdata/diff/comment/add_serial_sequence_comment_on_create/new.sql | Fixture “after” schema including sequence comment on implicitly created sequence. |
| testdata/diff/comment/add_serial_sequence_comment_on_create/diff.sql | Fixture expected diff SQL for SERIAL sequence comment on create. |
| testdata/diff/comment/add_sequence_comment/plan.txt | New fixture asserting sequence comment drift is detected in plan output. |
| testdata/diff/comment/add_sequence_comment/plan.sql | New fixture asserting COMMENT ON SEQUENCE SQL output. |
| testdata/diff/comment/add_sequence_comment/plan.json | New fixture asserting JSON step metadata for sequence comment changes. |
| testdata/diff/comment/add_sequence_comment/old.sql | Fixture “before” schema for sequence comment drift case. |
| testdata/diff/comment/add_sequence_comment/new.sql | Fixture “after” schema including sequence comments. |
| testdata/diff/comment/add_sequence_comment/diff.sql | Fixture expected diff SQL for sequence comments. |
| ir/queries/queries.sql.go | Extend inspector queries/rows to include sequence comments and trigger comment/enabled fields. |
| ir/ir.go | Add trigger disabled state to IR representation. |
| ir/inspector.go | Populate IR with sequence comments and trigger disabled state from catalog query results. |
| internal/diff/view.go | Emit trigger comment + enabled/disabled statements when modifying view triggers. |
| internal/diff/trigger.go | Emit trigger comment + enabled/disabled statements when creating triggers; add helper generators. |
| internal/diff/table.go | Treat trigger comment/disabled changes as trigger modifications and emit corresponding DDL. |
| internal/diff/sequence.go | Emit COMMENT ON SEQUENCE on create/alter when sequence comments are present/changed. |
| internal/diff/diff.go | Track SERIAL-owned sequences skipped from creation but needing comment emission to ensure idempotency. |
Files not reviewed (1)
- ir/queries/queries.sql.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1434
to
1442
| // Emit COMMENT ON TRIGGER for comment changes (including after structural recreate) | ||
| if commentChanged { | ||
| generateTriggerComment(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, DiffTypeTableTrigger, collector) | ||
| } | ||
|
|
||
| // Emit ENABLE/DISABLE TRIGGER for enabled state changes | ||
| if enabledChanged { | ||
| generateTriggerEnabledState(triggerDiff.New, td.Table.Schema, td.Table.Name, targetSchema, collector) | ||
| } |
Comment on lines
+439
to
+443
| generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) | ||
| } | ||
| if triggerDiff.New.Disabled { | ||
| generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, collector) | ||
| } |
Comment on lines
+455
to
+459
| generateTriggerComment(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, DiffTypeViewTrigger, collector) | ||
| } | ||
| if triggerDiff.New.Disabled { | ||
| generateTriggerEnabledState(triggerDiff.New, diff.New.Schema, diff.New.Name, targetSchema, collector) | ||
| } |
Comment on lines
+369
to
+373
| sql := fmt.Sprintf("ALTER TABLE %s %s TRIGGER %s;", tableName, state, ir.QuoteIdentifier(trigger.Name)) | ||
| context := &diffContext{ | ||
| Type: DiffTypeTableTrigger, | ||
| Operation: DiffOperationAlter, | ||
| Path: fmt.Sprintf("%s.%s.%s", schema, table, trigger.Name), |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four related gaps where pgschema silently ignored real schema properties,
causing silent drift between the declared model and the live database.
What changed
1. Trigger comments
pgschema had no awareness of
COMMENT ON TRIGGER. A comment declared inthe model was never applied; an unwanted comment on the live DB was never
flagged as drift.
ir/ir.go: addComment stringtoTriggerDefir/inspector.go+ir/queries/queries.sql.go: readpg_descriptionfor trigger OIDs
internal/diff/trigger.go: diff and emitCOMMENT ON TRIGGER ... ON ... IS '...'2. Trigger enabled/disabled state
pgschema had no awareness of
pg_trigger.tgenabled. A trigger disabled inthe model was never applied; a trigger enabled/disabled on the live DB was
never flagged as drift.
ir/ir.go: addEnabled booltoTriggerDefir/inspector.go+ir/queries/queries.sql.go: readtgenabledfrompg_triggerinternal/diff/trigger.go: diff and emitALTER TABLE ENABLE/DISABLE TRIGGER3. Sequence comments
pgschema had no awareness of
COMMENT ON SEQUENCE. A sequence comment inthe model was never applied; an unwanted comment on the live DB was never
flagged as drift.
ir/ir.go: addComment stringtoSequenceDefir/inspector.go+ir/queries/queries.sql.go: readpg_descriptionfor sequence OIDs
internal/diff/sequence.go: diff and emitCOMMENT ON SEQUENCE ... IS '...'4. Fix: SERIAL sequence comment idempotency
When a table with
BIGSERIAL/SERIALcolumns was created for the firsttime, pgschema correctly skipped emitting a standalone
CREATE SEQUENCE(because
CREATE TABLEcreates it implicitly). However, this alsoaccidentally skipped emitting any
COMMENT ON SEQUENCEdeclared in themodel. On the second run the sequence existed in the live DB, the comment
diff fired, and all sequence comments were re-applied indefinitely on every
subsequent run.
internal/diff/diff.go: track SERIAL-owned sequences that wereskipped from
addedSequencesbut carry a comment into a newaddedSerialSeqCommentslist; emit theirCOMMENT ON SEQUENCEafter allCREATE TABLEcalls completeVerified locally: two consecutive deploys against a freshly created wiser DB
— all 14 schemas report "No changes to apply" on the second run.
Tests
Four new fixtures, all passing:
testdata/diff/comment/add_trigger_comment/testdata/diff/comment/add_sequence_comment/testdata/diff/comment/add_serial_sequence_comment_on_create/← idempotency fixtestdata/diff/create_trigger/disable_trigger/